refactor: Migrate custom-user-status API to standardized format#39805
Closed
Makeepan-dev wants to merge 1 commit intoRocketChat:developfrom
Closed
refactor: Migrate custom-user-status API to standardized format#39805Makeepan-dev wants to merge 1 commit intoRocketChat:developfrom
Makeepan-dev wants to merge 1 commit intoRocketChat:developfrom
Conversation
Contributor
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
Contributor
WalkthroughThis change refactors custom user status API endpoints to use declarative AJV-based body validation and request-property types, consolidating endpoint type definitions in the rest-typings package instead of maintaining them locally alongside the API implementation. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip CodeRabbit can approve the review once all CodeRabbit's comments are resolved.Enable the |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
2 issues found across 3 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/meteor/app/api/server/v1/custom-user-status.ts">
<violation number="1" location="apps/meteor/app/api/server/v1/custom-user-status.ts:127">
P1: OpenAPI annotation for create endpoint has incorrect path and description copied from update endpoint</violation>
<violation number="2" location="apps/meteor/app/api/server/v1/custom-user-status.ts:335">
P2: Using `||` for update field fallback silently ignores explicit empty-string inputs, causing request payloads to be accepted but not applied.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| API.v1.addRoute( | ||
| /** | ||
| * @openapi | ||
| * /api/v1/custom-user-status.update: |
Contributor
There was a problem hiding this comment.
P1: OpenAPI annotation for create endpoint has incorrect path and description copied from update endpoint
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/app/api/server/v1/custom-user-status.ts, line 127:
<comment>OpenAPI annotation for create endpoint has incorrect path and description copied from update endpoint</comment>
<file context>
@@ -120,95 +122,237 @@ const customUserStatusEndpoints = API.v1.get(
-API.v1.addRoute(
+/**
+ * @openapi
+ * /api/v1/custom-user-status.update:
+ * post:
+ * description: Update an existing custom user status
</file context>
| await insertOrUpdateUserStatus(this.userId, { | ||
| _id, | ||
| name: name || customUserStatusToUpdate.name, | ||
| statusType: statusType || customUserStatusToUpdate.statusType, |
Contributor
There was a problem hiding this comment.
P2: Using || for update field fallback silently ignores explicit empty-string inputs, causing request payloads to be accepted but not applied.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/app/api/server/v1/custom-user-status.ts, line 335:
<comment>Using `||` for update field fallback silently ignores explicit empty-string inputs, causing request payloads to be accepted but not applied.</comment>
<file context>
@@ -120,95 +122,237 @@ const customUserStatusEndpoints = API.v1.get(
+ await insertOrUpdateUserStatus(this.userId, {
+ _id,
+ name: name || customUserStatusToUpdate.name,
+ statusType: statusType || customUserStatusToUpdate.statusType,
+ previousName: customUserStatusToUpdate.name,
+ previousStatusType: customUserStatusToUpdate.statusType,
</file context>
Suggested change
| statusType: statusType || customUserStatusToUpdate.statusType, | |
| statusType: statusType ?? customUserStatusToUpdate.statusType, |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
apps/meteor/app/api/server/v1/custom-user-status.ts (1)
6-15: Remove the migration notes from the route file.The commented-out import, the stale inline note on Line 327, and the mentor-only block at the end add no runtime value and will drift quickly once this lands.
As per coding guidelines:
**/*.{ts,tsx,js}: Avoid code comments in the implementation.Also applies to: 327-327, 352-357
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/api/server/v1/custom-user-status.ts` around lines 6 - 15, Remove the leftover migration comments and mentor-only notes from this route file: delete the commented import line "import { Match, check } from 'meteor/check';", drop the multi-line migration explanatory comment that mentions AJV/type removals, and remove the trailing mentor-only block at the end of the file so only runtime code and necessary imports remain; ensure no functional code (like import { Meteor }, API registration, or methods deleteCustomUserStatus/insertOrUpdateUserStatus) is altered when cleaning up the file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/meteor/app/api/server/v1/custom-user-status.ts`:
- Around line 125-158: The OpenAPI docs for custom-user-status are incorrect and
must match the actual handlers: update/documentation currently describes POST
/api/v1/custom-user-status.update and incorrectly requires name; change the
create and delete blocks so they use the correct paths/methods (e.g., POST
/api/v1/custom-user-status.create and POST or DELETE
/api/v1/custom-user-status.delete as implemented by the corresponding handlers)
and adjust the update block to only require _id (remove name from required) and
align properties with the AJV schema used by the update handler; update all
three OpenAPI blocks (create, update, delete) so their
operationIds/paths/methods and required fields match the real handler
implementations in custom-user-status.ts (the create, update, and delete handler
functions).
- Around line 332-337: Replace the || fallback for optional fields so explicit
empty-string values are preserved: in the call to insertOrUpdateUserStatus
(inside this user-status update block), use the nullish coalescing operator (??)
for name and statusType (e.g., name ?? customUserStatusToUpdate.name and
statusType ?? customUserStatusToUpdate.statusType) while leaving previousName
and previousStatusType as-is; this ensures clients can set empty strings without
being overridden by the old values.
In `@packages/rest-typings/src/v1/customUserStatus.ts`:
- Around line 9-74: This file re-introduces manual rest-typings artifacts that
violate Rule 7; remove the exported request/endpoint types and AJV validators
(CustomUserStatusCreateProps, CustomUserStatusDeleteProps,
CustomUserStatusUpdateProps, CustomUserStatusEndpoints,
isCustomUserStatusCreateProps, isCustomUserStatusDeleteProps,
isCustomUserStatusUpdateProps and the JSON schema constants) from
packages/rest-typings and instead keep the runtime contract and validators
colocated with the migrated endpoint; then expose the route types to consumers
via a local .d.ts augmentation in the consuming package as per the OpenAPI
migration pattern.
---
Nitpick comments:
In `@apps/meteor/app/api/server/v1/custom-user-status.ts`:
- Around line 6-15: Remove the leftover migration comments and mentor-only notes
from this route file: delete the commented import line "import { Match, check }
from 'meteor/check';", drop the multi-line migration explanatory comment that
mentions AJV/type removals, and remove the trailing mentor-only block at the end
of the file so only runtime code and necessary imports remain; ensure no
functional code (like import { Meteor }, API registration, or methods
deleteCustomUserStatus/insertOrUpdateUserStatus) is altered when cleaning up the
file.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d4caf39b-1e1d-4ae1-bdd5-518df203e8b7
📒 Files selected for processing (3)
apps/meteor/app/api/server/v1/custom-user-status.tspackages/rest-typings/src/index.tspackages/rest-typings/src/v1/customUserStatus.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: cubic · AI code reviewer
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
packages/rest-typings/src/index.tsapps/meteor/app/api/server/v1/custom-user-status.tspackages/rest-typings/src/v1/customUserStatus.ts
🧠 Learnings (14)
📓 Common learnings
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39676
File: .changeset/migrate-users-register-openapi.md:3-3
Timestamp: 2026-03-16T21:50:42.118Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs, removing endpoint types and validators from `rocket.chat/rest-typings` (e.g., `UserRegisterParamsPOST`, `/v1/users.register` entry) is the *required* migration pattern per RocketChat/Rocket.Chat-Open-API#150 Rule 7 ("No More rest-typings or Manual Typings"). The endpoint type is re-exposed via a module augmentation `.d.ts` file in the consuming package (e.g., `packages/web-ui-registration/src/users-register.d.ts`). This is NOT a breaking change — the correct changeset bump for `rocket.chat/rest-typings` in this scenario is `minor`, not `major`. Do not flag this as a breaking change during OpenAPI migration reviews.
Learnt from: smirk-dev
Repo: RocketChat/Rocket.Chat PR: 39625
File: apps/meteor/app/api/server/v1/push.ts:85-97
Timestamp: 2026-03-14T14:58:58.834Z
Learning: In RocketChat/Rocket.Chat, the `push.token` POST/DELETE endpoints in `apps/meteor/app/api/server/v1/push.ts` were already migrated to the chained router API pattern on `develop` prior to PR `#39625`. `cleanTokenResult` (which strips `authToken` and returns `PushTokenResult`) and `isPushTokenPOSTProps`/`isPushTokenDELETEProps` validators already exist on `develop`. PR `#39625` only migrates `push.get` and `push.info` to the chained pattern. Do not flag `cleanTokenResult` or `PushTokenResult` as newly introduced behavior-breaking changes when reviewing this PR.
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:09.561Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs for apps/meteor/app/api/server/v1 endpoints, maintainers prefer to avoid any logic changes; style-only cleanups (like removing inline comments) may be deferred to follow-ups to keep scope tight.
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39647
File: apps/meteor/app/api/server/v1/users.ts:710-757
Timestamp: 2026-03-15T14:31:28.969Z
Learning: In RocketChat/Rocket.Chat, the `UserCreateParamsPOST` type in `apps/meteor/app/api/server/v1/users.ts` (migrated from `packages/rest-typings/src/v1/users/UserCreateParamsPOST.ts`) intentionally has `fields: string` (non-optional) and `settings?: IUserSettings` without a corresponding AJV schema entry. This is a pre-existing divergence carried over verbatim from the original rest-typings source (PR `#39647`). Do not flag this type/schema misalignment during the OpenAPI migration review — it is tracked as a separate follow-up fix.
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39647
File: apps/meteor/app/api/server/v1/users.ts:891-899
Timestamp: 2026-03-15T14:31:23.493Z
Learning: In RocketChat/Rocket.Chat, `IUser.inactiveReason` in `packages/core-typings/src/IUser.ts` is typed as `'deactivated' | 'pending_approval' | 'idle_too_long'` (optional, no `null`), but the database stores `null` for newly created users. The Typia-generated `$ref: '#/components/schemas/IUser'` schema therefore correctly rejects `null` for `inactiveReason`. This causes the test "should create a new user with default roles" to fail when response validation is active (TEST_MODE). The fix is to add `| null` to `inactiveReason` in core-typings and rebuild Typia schemas in a separate PR. Do not flag this test failure as a bug introduced by the users.create OpenAPI migration (PR `#39647`). Do not suggest inlining a custom schema to work around it, as migration rules require using `$ref` when a Typia schema exists.
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39676
File: apps/meteor/app/api/server/v1/users.ts:862-869
Timestamp: 2026-03-16T23:33:15.721Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs (e.g., PR `#39676` for users.register in apps/meteor/app/api/server/v1/users.ts), calls to `this.parseJsonQuery()` inside migrated handlers are intentionally preserved without adding a corresponding `query` AJV schema to the route options. Adding query-param schemas for the `fields`/`sort`/`query` parameters consumed by `parseJsonQuery()` is a separate cross-cutting concern shared by many endpoints (e.g., users.create, users.update, users.list) and is explicitly out of scope for individual endpoint migration PRs. Do not flag the absence of a `query` schema for `parseJsonQuery()` usage as a violation of OpenAPI/AJV contract during migration reviews.
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 39414
File: apps/meteor/app/api/server/v1/rooms.ts:1241-1297
Timestamp: 2026-03-10T08:13:52.153Z
Learning: In the RocketChat/Rocket.Chat OpenAPI migration PRs for endpoints under apps/meteor/app/api/server/v1/rooms.ts, the pattern `ajv.compile<void>({...})` is intentionally used for the 200 response schema even when the endpoint returns `{ success: true }`. This is an established convention across all migrated endpoints (rooms.leave, rooms.favorite, rooms.delete, rooms.muteUser, rooms.unmuteUser). Do not flag this as a type mismatch during reviews of these migration PRs.
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 39553
File: apps/meteor/app/api/server/v1/stats.ts:98-117
Timestamp: 2026-03-20T13:52:26.083Z
Learning: In `apps/meteor/app/api/server/v1/stats.ts`, the `statistics.telemetry` POST endpoint intentionally has no `body` AJV schema in its route options. The proper request body shape (a `params` array of telemetry event objects) has not been formally defined yet, so body validation is deferred to a follow-up. Do not flag the missing body schema for this endpoint during OpenAPI migration reviews.
📚 Learning: 2026-03-16T21:50:42.118Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39676
File: .changeset/migrate-users-register-openapi.md:3-3
Timestamp: 2026-03-16T21:50:42.118Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs, removing endpoint types and validators from `rocket.chat/rest-typings` (e.g., `UserRegisterParamsPOST`, `/v1/users.register` entry) is the *required* migration pattern per RocketChat/Rocket.Chat-Open-API#150 Rule 7 ("No More rest-typings or Manual Typings"). The endpoint type is re-exposed via a module augmentation `.d.ts` file in the consuming package (e.g., `packages/web-ui-registration/src/users-register.d.ts`). This is NOT a breaking change — the correct changeset bump for `rocket.chat/rest-typings` in this scenario is `minor`, not `major`. Do not flag this as a breaking change during OpenAPI migration reviews.
Applied to files:
packages/rest-typings/src/index.tsapps/meteor/app/api/server/v1/custom-user-status.tspackages/rest-typings/src/v1/customUserStatus.ts
📚 Learning: 2026-03-15T14:31:28.969Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39647
File: apps/meteor/app/api/server/v1/users.ts:710-757
Timestamp: 2026-03-15T14:31:28.969Z
Learning: In RocketChat/Rocket.Chat, the `UserCreateParamsPOST` type in `apps/meteor/app/api/server/v1/users.ts` (migrated from `packages/rest-typings/src/v1/users/UserCreateParamsPOST.ts`) intentionally has `fields: string` (non-optional) and `settings?: IUserSettings` without a corresponding AJV schema entry. This is a pre-existing divergence carried over verbatim from the original rest-typings source (PR `#39647`). Do not flag this type/schema misalignment during the OpenAPI migration review — it is tracked as a separate follow-up fix.
Applied to files:
packages/rest-typings/src/index.tsapps/meteor/app/api/server/v1/custom-user-status.tspackages/rest-typings/src/v1/customUserStatus.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
packages/rest-typings/src/index.tsapps/meteor/app/api/server/v1/custom-user-status.tspackages/rest-typings/src/v1/customUserStatus.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
packages/rest-typings/src/index.tsapps/meteor/app/api/server/v1/custom-user-status.tspackages/rest-typings/src/v1/customUserStatus.ts
📚 Learning: 2026-02-24T19:09:01.522Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:01.522Z
Learning: In Rocket.Chat OpenAPI migration PRs for endpoints under apps/meteor/app/api/server/v1, avoid introducing logic changes. Only perform scope-tight changes that preserve behavior; style-only cleanups (e.g., removing inline comments) may be deferred to follow-ups to keep the migration PR focused.
Applied to files:
apps/meteor/app/api/server/v1/custom-user-status.ts
📚 Learning: 2026-03-14T14:58:58.834Z
Learnt from: smirk-dev
Repo: RocketChat/Rocket.Chat PR: 39625
File: apps/meteor/app/api/server/v1/push.ts:85-97
Timestamp: 2026-03-14T14:58:58.834Z
Learning: In RocketChat/Rocket.Chat, the `push.token` POST/DELETE endpoints in `apps/meteor/app/api/server/v1/push.ts` were already migrated to the chained router API pattern on `develop` prior to PR `#39625`. `cleanTokenResult` (which strips `authToken` and returns `PushTokenResult`) and `isPushTokenPOSTProps`/`isPushTokenDELETEProps` validators already exist on `develop`. PR `#39625` only migrates `push.get` and `push.info` to the chained pattern. Do not flag `cleanTokenResult` or `PushTokenResult` as newly introduced behavior-breaking changes when reviewing this PR.
Applied to files:
apps/meteor/app/api/server/v1/custom-user-status.ts
📚 Learning: 2026-03-12T10:26:26.697Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 39340
File: apps/meteor/app/api/server/v1/im.ts:1349-1398
Timestamp: 2026-03-12T10:26:26.697Z
Learning: In `apps/meteor/app/api/server/v1/im.ts` (PR `#39340`), the `DmEndpoints` type intentionally includes temporary stub entries for `/v1/im.kick`, `/v1/dm.kick`, `/v1/im.leave`, and `/v1/dm.leave` (using `DmKickProps` and `DmLeaveProps`) even though no route handlers exist for them yet. These stubs were added to preserve type compatibility after removing the original `DmLeaveProps` and related files. They are planned for cleanup in a follow-up PR. Do not flag these as missing implementations when reviewing this file until the follow-up is merged.
Applied to files:
apps/meteor/app/api/server/v1/custom-user-status.ts
📚 Learning: 2026-03-16T23:33:15.721Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39676
File: apps/meteor/app/api/server/v1/users.ts:862-869
Timestamp: 2026-03-16T23:33:15.721Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs (e.g., PR `#39676` for users.register in apps/meteor/app/api/server/v1/users.ts), calls to `this.parseJsonQuery()` inside migrated handlers are intentionally preserved without adding a corresponding `query` AJV schema to the route options. Adding query-param schemas for the `fields`/`sort`/`query` parameters consumed by `parseJsonQuery()` is a separate cross-cutting concern shared by many endpoints (e.g., users.create, users.update, users.list) and is explicitly out of scope for individual endpoint migration PRs. Do not flag the absence of a `query` schema for `parseJsonQuery()` usage as a violation of OpenAPI/AJV contract during migration reviews.
Applied to files:
apps/meteor/app/api/server/v1/custom-user-status.tspackages/rest-typings/src/v1/customUserStatus.ts
📚 Learning: 2026-03-20T13:52:26.083Z
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 39553
File: apps/meteor/app/api/server/v1/stats.ts:98-117
Timestamp: 2026-03-20T13:52:26.083Z
Learning: In `apps/meteor/app/api/server/v1/stats.ts`, the `statistics.telemetry` POST endpoint intentionally has no `body` AJV schema in its route options. The proper request body shape (a `params` array of telemetry event objects) has not been formally defined yet, so body validation is deferred to a follow-up. Do not flag the missing body schema for this endpoint during OpenAPI migration reviews.
Applied to files:
apps/meteor/app/api/server/v1/custom-user-status.ts
📚 Learning: 2026-02-25T20:10:16.987Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38913
File: packages/ddp-client/src/legacy/types/SDKLegacy.ts:34-34
Timestamp: 2026-02-25T20:10:16.987Z
Learning: In the RocketChat/Rocket.Chat monorepo, packages/ddp-client and apps/meteor do not use TypeScript project references. Module augmentations in apps/meteor (e.g., declare module 'rocket.chat/rest-typings') are not visible when compiling packages/ddp-client in isolation, which is why legacy SDK methods that depend on OperationResult types for OpenAPI-migrated endpoints must remain commented out.
Applied to files:
apps/meteor/app/api/server/v1/custom-user-status.ts
📚 Learning: 2026-03-10T08:13:52.153Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 39414
File: apps/meteor/app/api/server/v1/rooms.ts:1241-1297
Timestamp: 2026-03-10T08:13:52.153Z
Learning: In the RocketChat/Rocket.Chat OpenAPI migration PRs for endpoints under apps/meteor/app/api/server/v1/rooms.ts, the pattern `ajv.compile<void>({...})` is intentionally used for the 200 response schema even when the endpoint returns `{ success: true }`. This is an established convention across all migrated endpoints (rooms.leave, rooms.favorite, rooms.delete, rooms.muteUser, rooms.unmuteUser). Do not flag this as a type mismatch during reviews of these migration PRs.
Applied to files:
apps/meteor/app/api/server/v1/custom-user-status.tspackages/rest-typings/src/v1/customUserStatus.ts
📚 Learning: 2026-03-15T14:31:23.493Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39647
File: apps/meteor/app/api/server/v1/users.ts:891-899
Timestamp: 2026-03-15T14:31:23.493Z
Learning: In RocketChat/Rocket.Chat, `IUser.inactiveReason` in `packages/core-typings/src/IUser.ts` is typed as `'deactivated' | 'pending_approval' | 'idle_too_long'` (optional, no `null`), but the database stores `null` for newly created users. The Typia-generated `$ref: '#/components/schemas/IUser'` schema therefore correctly rejects `null` for `inactiveReason`. This causes the test "should create a new user with default roles" to fail when response validation is active (TEST_MODE). The fix is to add `| null` to `inactiveReason` in core-typings and rebuild Typia schemas in a separate PR. Do not flag this test failure as a bug introduced by the users.create OpenAPI migration (PR `#39647`). Do not suggest inlining a custom schema to work around it, as migration rules require using `$ref` when a Typia schema exists.
Applied to files:
apps/meteor/app/api/server/v1/custom-user-status.tspackages/rest-typings/src/v1/customUserStatus.ts
📚 Learning: 2026-02-23T17:53:06.802Z
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 35995
File: apps/meteor/app/api/server/v1/rooms.ts:1107-1112
Timestamp: 2026-02-23T17:53:06.802Z
Learning: During PR reviews that touch endpoint files under apps/meteor/app/api/server/v1, enforce strict scope: if a PR targets a specific endpoint (e.g., rooms.favorite), do not propose changes to unrelated endpoints (e.g., rooms.invite) unless maintainers explicitly request them. Focus feedback on the touched endpoint's behavior, API surface, and related tests; avoid broad cross-endpoint changes in the same PR unless requested.
Applied to files:
apps/meteor/app/api/server/v1/custom-user-status.ts
Comment on lines
+125
to
+158
| /** | ||
| * @openapi | ||
| * /api/v1/custom-user-status.update: | ||
| * post: | ||
| * description: Update an existing custom user status | ||
| * security: | ||
| * - cookieAuth: [] | ||
| * - x-user-id: [] | ||
| * - x-auth-token: [] | ||
| * requestBody: | ||
| * content: | ||
| * application/json: | ||
| * schema: | ||
| * type: object | ||
| * properties: | ||
| * _id: | ||
| * type: string | ||
| * name: | ||
| * type: string | ||
| * statusType: | ||
| * type: string | ||
| * required: | ||
| * - _id | ||
| * - name | ||
| * responses: | ||
| * 200: | ||
| * description: The updated custom user status | ||
| * content: | ||
| * application/json: | ||
| * schema: | ||
| * $ref: '#/components/schemas/ApiSuccessV1' | ||
| * default: | ||
| * $ref: '#/components/schemas/ApiErrorsV1' | ||
| */ |
Contributor
There was a problem hiding this comment.
The OpenAPI blocks are out of sync with the handlers.
The create and delete handlers are both documented as POST /api/v1/custom-user-status.update, so the generated spec will miss the real create/delete operations. The update block is also stricter than the actual AJV contract because it marks name as required even though only _id is required.
Also applies to: 201-234, 264-297
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/meteor/app/api/server/v1/custom-user-status.ts` around lines 125 - 158,
The OpenAPI docs for custom-user-status are incorrect and must match the actual
handlers: update/documentation currently describes POST
/api/v1/custom-user-status.update and incorrectly requires name; change the
create and delete blocks so they use the correct paths/methods (e.g., POST
/api/v1/custom-user-status.create and POST or DELETE
/api/v1/custom-user-status.delete as implemented by the corresponding handlers)
and adjust the update block to only require _id (remove name from required) and
align properties with the AJV schema used by the update handler; update all
three OpenAPI blocks (create, update, delete) so their
operationIds/paths/methods and required fields match the real handler
implementations in custom-user-status.ts (the create, update, and delete handler
functions).
Comment on lines
+332
to
+337
| await insertOrUpdateUserStatus(this.userId, { | ||
| _id, | ||
| name: name || customUserStatusToUpdate.name, | ||
| statusType: statusType || customUserStatusToUpdate.statusType, | ||
| previousName: customUserStatusToUpdate.name, | ||
| previousStatusType: customUserStatusToUpdate.statusType, |
Contributor
There was a problem hiding this comment.
Use ?? here to preserve empty-string updates.
|| treats '' as “not provided”, so a client can no longer clear statusType back to the empty-string value that create already uses. ?? keeps the old optional-field fallback without changing explicit empty-string inputs.
🔧 Proposed fix
await insertOrUpdateUserStatus(this.userId, {
_id,
- name: name || customUserStatusToUpdate.name,
- statusType: statusType || customUserStatusToUpdate.statusType,
+ name: name ?? customUserStatusToUpdate.name,
+ statusType: statusType ?? customUserStatusToUpdate.statusType,
previousName: customUserStatusToUpdate.name,
previousStatusType: customUserStatusToUpdate.statusType,
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| await insertOrUpdateUserStatus(this.userId, { | |
| _id, | |
| name: name || customUserStatusToUpdate.name, | |
| statusType: statusType || customUserStatusToUpdate.statusType, | |
| previousName: customUserStatusToUpdate.name, | |
| previousStatusType: customUserStatusToUpdate.statusType, | |
| await insertOrUpdateUserStatus(this.userId, { | |
| _id, | |
| name: name ?? customUserStatusToUpdate.name, | |
| statusType: statusType ?? customUserStatusToUpdate.statusType, | |
| previousName: customUserStatusToUpdate.name, | |
| previousStatusType: customUserStatusToUpdate.statusType, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/meteor/app/api/server/v1/custom-user-status.ts` around lines 332 - 337,
Replace the || fallback for optional fields so explicit empty-string values are
preserved: in the call to insertOrUpdateUserStatus (inside this user-status
update block), use the nullish coalescing operator (??) for name and statusType
(e.g., name ?? customUserStatusToUpdate.name and statusType ??
customUserStatusToUpdate.statusType) while leaving previousName and
previousStatusType as-is; this ensures clients can set empty strings without
being overridden by the old values.
Comment on lines
+9
to
+74
| export type CustomUserStatusCreateProps = { | ||
| name: string; | ||
| statusType?: string; | ||
| }; | ||
|
|
||
| export type CustomUserStatusDeleteProps = { | ||
| customUserStatusId: string; | ||
| }; | ||
|
|
||
| export type CustomUserStatusUpdateProps = { | ||
| _id: string; | ||
| name?: string; | ||
| statusType?: string; | ||
| }; | ||
|
|
||
| export type CustomUserStatusEndpoints = { | ||
| '/v1/custom-user-status.create': { | ||
| POST: (params: { name: string; statusType?: string }) => { | ||
| POST: (params: CustomUserStatusCreateProps) => { | ||
| customUserStatus: ICustomUserStatus; | ||
| }; | ||
| }; | ||
| '/v1/custom-user-status.delete': { | ||
| POST: (params: { customUserStatusId: string }) => void; | ||
| POST: (params: CustomUserStatusDeleteProps) => void; | ||
| }; | ||
| '/v1/custom-user-status.update': { | ||
| POST: (params: { _id: string; name?: string; statusType?: string }) => { | ||
| POST: (params: CustomUserStatusUpdateProps) => { | ||
| customUserStatus: ICustomUserStatus; | ||
| }; | ||
| }; | ||
| }; | ||
|
|
||
| const customUserStatusCreatePropsSchema: JSONSchemaType<CustomUserStatusCreateProps> = { | ||
| type: 'object', | ||
| properties: { | ||
| name: { type: 'string' }, | ||
| statusType: { type: 'string', nullable: true }, | ||
| }, | ||
| required: ['name'], | ||
| additionalProperties: false, | ||
| }; | ||
|
|
||
| export const isCustomUserStatusCreateProps = ajv.compile<CustomUserStatusCreateProps>(customUserStatusCreatePropsSchema); | ||
|
|
||
| const customUserStatusDeletePropsSchema: JSONSchemaType<CustomUserStatusDeleteProps> = { | ||
| type: 'object', | ||
| properties: { | ||
| customUserStatusId: { type: 'string' }, | ||
| }, | ||
| required: ['customUserStatusId'], | ||
| additionalProperties: false, | ||
| }; | ||
|
|
||
| export const isCustomUserStatusDeleteProps = ajv.compile<CustomUserStatusDeleteProps>(customUserStatusDeletePropsSchema); | ||
|
|
||
| const customUserStatusUpdatePropsSchema: JSONSchemaType<CustomUserStatusUpdateProps> = { | ||
| type: 'object', | ||
| properties: { | ||
| _id: { type: 'string' }, | ||
| name: { type: 'string', nullable: true }, | ||
| statusType: { type: 'string', nullable: true }, | ||
| }, | ||
| required: ['_id'], | ||
| additionalProperties: false, | ||
| }; | ||
|
|
||
| export const isCustomUserStatusUpdateProps = ajv.compile<CustomUserStatusUpdateProps>(customUserStatusUpdatePropsSchema); |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Please follow Rule 7 here instead of re-adding manual rest-typings entries.
This moves the migrated contract back into packages/rest-typings by adding new request props, endpoint signatures, and compiled validators there. For the OpenAPI migrations we’ve been standardizing on the opposite direction: keep the runtime contract next to the migrated endpoint and re-expose the route type via a local .d.ts augmentation in the consuming package.
Based on learnings: In RocketChat/Rocket.Chat OpenAPI migration PRs, removing endpoint types and validators from rocket.chat/rest-typings is the required migration pattern per RocketChat/Rocket.Chat-Open-API#150 Rule 7 ("No More rest-typings or Manual Typings").
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/rest-typings/src/v1/customUserStatus.ts` around lines 9 - 74, This
file re-introduces manual rest-typings artifacts that violate Rule 7; remove the
exported request/endpoint types and AJV validators (CustomUserStatusCreateProps,
CustomUserStatusDeleteProps, CustomUserStatusUpdateProps,
CustomUserStatusEndpoints, isCustomUserStatusCreateProps,
isCustomUserStatusDeleteProps, isCustomUserStatusUpdateProps and the JSON schema
constants) from packages/rest-typings and instead keep the runtime contract and
validators colocated with the migrated endpoint; then expose the route types to
consumers via a local .d.ts augmentation in the consuming package as per the
OpenAPI migration pattern.
Member
|
Hey, thanks for the contribution! 🙏 I'm closing this PR because the endpoint(s) covered here have already been migrated as part of a larger batch migration I'm working on (see #39820). I decided to go with a mass migration approach because reviewing individual PRs for each endpoint was taking too much of my time. That said, you're more than welcome to help by reviewing and testing the changes in #39820 to make sure everything is working correctly. Your input would be really valuable! Thanks again for your effort! 🚀 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed changes
Migrated the
custom-user-statusAPI endpoints from the legacyaddRoutepattern to the new standardized format for better type safety and automatic documentation.Key technical changes:
@rocket.chat/rest-typings.check()validation with the built-invalidateproperty.anyusages and leveraging shared core typings.Issue(s)
Related to the GSoC 2026 Migration project.
Steps to test or reproduce
/api/v1/custom-user-status.listvia API client.Further comments
This migration follows the established architectural patterns used in other successfully migrated endpoints (like channels/rooms). No breaking changes were made to the request or response payloads to ensure 100% backward compatibility for mobile and desktop clients.
Note: Local E2E tests were limited by local hardware resources (ENOMEM), but the code has been verified via local linting and matches the project's standardized migration patterns.
Summary by CodeRabbit
Release Notes